READY: DHTCommunity #3642
READY: DHTCommunity #3642
Conversation
|
retest this please |
egbertbouman
force-pushed
the
dht
branch
3 times, most recently
from
2d23318
to
6c83a00
Compare
Tribler/community/dht/community.py
Outdated
|
|
||
| _, _, payload = self._ez_unpack_auth(PingResponsePayload, data) | ||
|
|
||
| if not self.request_cache.has(u'request', payload.identifier): |
There was a problem hiding this comment.
Log an error here please.
|
|
||
| @twisted_wrapper | ||
| def test_caching(self): | ||
| # Add a third node |
There was a problem hiding this comment.
I guess it's easier to use https://github.com/Tribler/py-ipv8/blob/master/ipv8/test/base.py#L93, instead of creating a new MockIPv8 instance?
There was a problem hiding this comment.
Oh, I missed that function. Thanks!
| yield self.nodes[0].overlay.find_values('\x00' * 20) | ||
| yield self.deliver_messages() | ||
|
|
||
| self.assertTrue(bool(self.nodes[1].overlay.storage.get('\x00' * 20))) |
There was a problem hiding this comment.
Maybe check for the exact string instead? (self.assertEquals(...))
| self.rtt = 0 | ||
|
|
||
|
|
||
| class TestNode(unittest.TestCase): |
There was a problem hiding this comment.
Please subclass from AbstractServer instead so you get additional debug features (there are some more classes in this PR who inherit from the same class).
| self.assertFalse(self.bucket.owns(pad_and_convert('11'))) | ||
|
|
||
| def test_get(self): | ||
| self.bucket.nodes[1]= 'node' |
There was a problem hiding this comment.
Minor comment: add a space before the = sign.
|
|
||
| class TestRoutingTable(unittest.TestCase): | ||
| """ | ||
| Class to test the routing table |
There was a problem hiding this comment.
Please be consistent and either add a docstring under every class definition or not 👍
| for bucket in self.routing_table.trie.values(): | ||
| for node in bucket.nodes.values(): | ||
| if node.last_response + PING_INTERVAL <= now: | ||
| deferreds.append(self.ping(node).addErrback(lambda _: None)) |
There was a problem hiding this comment.
It is correct that you do not want to do anything when a ping to a node fails?
There was a problem hiding this comment.
It is. When a ping fails, Node.failed will be incremented, which will eventually lead to the Node being removed from the routing table.
Tribler/community/dht/provider.py
Outdated
| def lookup(self, info_hash, cb): | ||
| def callback(values): | ||
| addresses = [] | ||
| for v in values: |
There was a problem hiding this comment.
Please use a more descriptive name than v
| from threading import RLock | ||
| from socket import inet_aton | ||
|
|
||
| import datrie |
There was a problem hiding this comment.
Did you check whether datrie is available in the debian repositories? Also, please update the documentation and add instructions on how to install this new dependency 👍
There was a problem hiding this comment.
Yup, it's in the repositories, and I just updated the docs.
| return distance(self.id, other_node.id) | ||
|
|
||
|
|
||
| class Bucket(object): |
There was a problem hiding this comment.
Could you please add a small docstring under the most important classes to explain what it represents?
twisted/plugins/dht_plugin.py
Outdated
| ] | ||
|
|
||
|
|
||
| class DHTServiceMaker(object): |
There was a problem hiding this comment.
Two comments:
- I noticed that we are reusing so much of the original Tribler plugin which leads to many different twistd plugins with duplicate code. At one point, we should merge them all together and just provide the Tribler twistd plugin (with additional parameters to run DHT/market etc).
- When should this plugin be used?
There was a problem hiding this comment.
I used it mostly for testing, but we could also use it to run DHT routers. Since we don't really need it I'm OK with removing it.
There was a problem hiding this comment.
Ok, can you remove this file then? 👍
|
retest this please |
No description provided.